Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

haskell-modules/generic-builder.nix: work around libc++abi issue #266172

Merged
merged 1 commit into from
Nov 11, 2023

Conversation

reckenrode
Copy link
Contributor

@reckenrode reckenrode commented Nov 8, 2023

Description of changes

Fixes failing build on staging-next #263535 plus whatever else was broken in nixpkgs-review.

This is a workaround until cc-wrapper is fixed to link libc++abi even when invoked as just clang. Other approaches were investigated (modifying haskell-modules/generic-builder.nix), but they would result in mass rebuilds.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 8, 2023
@reckenrode reckenrode changed the title Fix a bunch of libc++abi failures on staging-next Fix several libc++abi failures on staging-next Nov 8, 2023
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 8, 2023
@cdepillabout
Copy link
Member

I tried an approach that modified haskell-modules/generic-builder.nix, but that’s not really the right place, and it resulted in tons of rebuilds

I haven't been following these clang changes. Could you be more specific about where would be the right place to make these fixes?

For instance, if you don't fix haskell-modules/generic-builder.nix, will there be lots of downstream breakages in end-user's Haskell projects?

Once cc-wrapper is fixed so clang includes libc++abi automatically even when it’s invoked as just clang, these commits can be reverted.

Is there an issue/PR you could link to in the code that made it easier to track when this happens? (Maybe it is #166205?)

Also, if these commits are meant to just all be reverted soon, it might make it easier for us if all of the changes in this PR were included in a single commit.

@reckenrode
Copy link
Contributor Author

I haven't been following these clang changes. Could you be more specific about where would be the right place to make these fixes?

The right place is cc-wrapper. This problem affects any program that invokes clang to build C++ code. See also: #216047 and #150655.

My primary concern with modifying haskell-modules/generic-builder.nix until cc-wrapper is fixed is the number of rebuilds it will cause.

For instance, if you don't fix haskell-modules/generic-builder.nix, will there be lots of downstream breakages in end-user's Haskell projects?

Depending on which modules are used, potentially.

Should I go ahead with modifying haskell-modules/generic-builder.nix even if it’s going to result in rebuilding effectively everything? I can target the change so it doesn’t affect Linux, but there’s no way of avoiding mass rebuilds on Darwin.

Once cc-wrapper is fixed so clang includes libc++abi automatically even when it’s invoked as just clang, these commits can be reverted.

Is there an issue/PR you could link to in the code that made it easier to track when this happens? (Maybe it is #166205?)

I pinged the Nixpkgs/NixOS Contributions room on Matrix to see if anyone is working on a fix. I thought someone was, but I don’t recall who offhand and don’t think there is an issue or draft PR tracking that work.

Also, if these commits are meant to just all be reverted soon, it might make it easier for us if all of the changes in this PR were included in a single commit.

Depending on whether the preference is to do a localized fix here or fix it Haskell-wide at the cost of rebuilds, I can squash or push that alternative instead.

@reckenrode reckenrode force-pushed the clang16-fixes-batch5 branch from 06d4bc6 to b0fac4d Compare November 8, 2023 14:35
@reckenrode
Copy link
Contributor Author

reckenrode commented Nov 8, 2023

Pinging @toonn for awareness since he’s working on the cc-wrapper changes.

@cdepillabout I went ahead and squashed the commits as requested.

I skimmed the list of problematic dependencies in the Nix Tools report for the latest staging-next evaluation. The other Haskell issues don’t appear to be related to this one. It doesn’t seem not worth the mass rebuilds to use a more general fix.

@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 8, 2023
@cdepillabout
Copy link
Member

My primary concern with modifying haskell-modules/generic-builder.nix until cc-wrapper is fixed is the number of rebuilds it will cause.

The other Haskell issues don’t appear to be related to this one. It doesn’t seem not worth the mass rebuilds to use a more general fix.

The thing about the Haskell stuff in Nixpkgs is that it is commonly used by end users, to build their own packages not included in Nixpkgs.

If there is a way we can fix it generally, so that individual packages/users don't have to worry about it, I'd say we should do it.

My personal opinion is that it is more important to not break end-users' code, rather than trying to minimalize rebuilds on Hydra.

@reckenrode
Copy link
Contributor Author

Sure, I can do that. Thank you for clarifying. I’ve been erring on the side of avoiding rebuilds if possible, but I can put the fix in haskell-modules/generic-builder.nix so no one’s code breaks since that is an understandably higher priority.

(I assume that pushing the fix to staging instead is undesirable because there would be a risk of end-user code breaking before the next staging makes it way to master.)

I’ve tried to avoid rebuilds on Linux with my change. It’s narrowly targeted at clang stdenvs and only those using libc++, which Linux does not use by default. Once I confirm Cachix builds, I’ll update this PR.

Work around clang’s not linking libc++abi when invoked as `clang` or
`cc` even when it’s needed to link a C++ library or C++ code.

See NixOS#166205.
@reckenrode reckenrode force-pushed the clang16-fixes-batch5 branch from b0fac4d to 8164b19 Compare November 9, 2023 02:56
@reckenrode reckenrode changed the title Fix several libc++abi failures on staging-next haskell-modules/generic-builder.nix: work around libc++abi issue Nov 9, 2023
@reckenrode
Copy link
Contributor Author

reckenrode commented Nov 9, 2023

I pushed the new patch and updated the PR’s title to reflect the change.

Cachix successfully built on both aarch64-darwin and x86_64-darwin with just this patch.

@cdepillabout
Copy link
Member

@reckenrode Thanks!

This seems reasonable. Maybe @sternenseemann could do a final check, and we could merge in if he signs off on this.


# Ensure libc++abi is linked even when clang is invoked as just `clang` or `cc`.
# Works around https://github.com/NixOS/nixpkgs/issues/166205.
# This can be dropped once a fix has been committed to cc-wrapper.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has a PR with the proposed fix to cc-wrapper been opened?

Copy link
Contributor Author

@reckenrode reckenrode Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toonn is working on the fix to cc-wrapper. I don’t believe he’s opened a PR for it yet.

@ghost
Copy link

ghost commented Nov 9, 2023

The Hydra log from the top of this issue has been deleted so I can't see the failure.

I suspect the root cause is that Darwin's libcxx doesn't have isLLVM=true even though it is LLVM-based:

+ optionalString (libcxx.isLLVM or false) ''
echo "-isystem ${lib.getDev libcxx}/include/c++/v1" >> $out/nix-support/libcxx-cxxflags
echo "-isystem ${lib.getDev libcxx.cxxabi}/include/c++/v1" >> $out/nix-support/libcxx-cxxflags
echo "-stdlib=libc++" >> $out/nix-support/libcxx-ldflags
echo "-l${libcxx.cxxabi.libName}" >> $out/nix-support/libcxx-ldflags
''

@reckenrode
Copy link
Contributor Author

reckenrode commented Nov 9, 2023

I suspect the root cause is that Darwin's libcxx doesn't have isLLVM=true even though it is LLVM-based:

As I understand it, Darwin is not isLLVM because it doesn’t use enough of the toolchain. Historically, it used clang and libc++. That was it pretty much it. This release (23.11), it’s also using whatever LLVM tools that are drop-in replacements for those from cctools, but that excludes several due to limited compatibility. In particular, it’s not using lld because lld isn’t fully capable of replacing ld64. I’d like to explore using lld 18 at least to build the stdenv for 24.11, but I’m still not sure whether it will be compatible enough to be the default linker on Darwin. (The fact that ld-prime is also not very compatible may encourage people to be more flexible about linking on Darwin, but that remains to be seen.)

+ optionalString (libcxx.isLLVM or false) ''
echo "-isystem ${lib.getDev libcxx}/include/c++/v1" >> $out/nix-support/libcxx-cxxflags
echo "-isystem ${lib.getDev libcxx.cxxabi}/include/c++/v1" >> $out/nix-support/libcxx-cxxflags
echo "-stdlib=libc++" >> $out/nix-support/libcxx-ldflags
echo "-l${libcxx.cxxabi.libName}" >> $out/nix-support/libcxx-ldflags
''

Darwin’s nix-support/libcxx-ldflags already includes both -stdlib=libc++ and -lc++abi. It can build and link C++ code fine when it is invoked as clang++, but it doesn’t include the C++ stuff when it is invoked as just clang. Unfortunately, unwrapped clang does, and some packages (such as Haskell and Bazel) expect to invoke clang and have it do that when building and linking to C++ code.

For example, given (and pardon my C++98):

#include <iostream>

int main() {
    std::cout << "Hello, GitHub!" << std::endl;
}

Compiling it with Xcode’s clang: clang -o test test.cc -lc++ results in a working binary. Doing the same with the wrapped clang from nixpkgs results in:

test.cc:1:10: fatal error: 'iostream' file not found
#include <iostream>
         ^~~~~~~~~~
1 error generated.

Which is because it’s ignoring the contents of nix-support/libcxx-cxxflags. If I specify the contents of that file on the command-line (or invoke it as clang++), it fails like this (and this is the kind of error affecting Haskell and Bazel):

Undefined symbols for architecture arm64:
  "std::terminate()", referenced from:
      ___clang_call_terminate in test-9e5058.o
  "___cxa_begin_catch", referenced from:
      std::__1::basic_ostream<char, std::__1::char_traits<char>>& std::__1::__put_character_sequence[abi:v160006]<char, std::__1::char_traits<char>>(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, char const*, unsigned long) in test-9e5058.o
      ___clang_call_terminate in test-9e5058.o
  "___cxa_end_catch", referenced from:
      std::__1::basic_ostream<char, std::__1::char_traits<char>>& std::__1::__put_character_sequence[abi:v160006]<char, std::__1::char_traits<char>>(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, char const*, unsigned long) in test-9e5058.o
  "___gxx_personality_v0", referenced from:
      std::__1::basic_ostream<char, std::__1::char_traits<char>>& std::__1::__put_character_sequence[abi:v160006]<char, std::__1::char_traits<char>>(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, char const*, unsigned long) in test-9e5058.o
      std::__1::char_traits<char>::length(char const*) in test-9e5058.o
      std::__1::ostreambuf_iterator<char, std::__1::char_traits<char>> std::__1::__pad_and_output[abi:v160006]<char, std::__1::char_traits<char>>(std::__1::ostreambuf_iterator<char, std::__1::char_traits<char>>, char const*, char const*, char const*, std::__1::ios_base&, char) in test-9e5058.o
      std::__1::ostreambuf_iterator<char, std::__1::char_traits<char>>::ostreambuf_iterator[abi:v160006](std::__1::basic_ostream<char, std::__1::char_traits<char>>&) in test-9e5058.o
      std::__1::basic_ios<char, std::__1::char_traits<char>>::widen[abi:v160006](char) const in test-9e5058.o
ld: symbol(s) not found for architecture arm64
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)

If I add -lc++abi, it finally builds and links.

@wegank wegank mentioned this pull request Nov 10, 2023
13 tasks
@vcunat
Copy link
Member

vcunat commented Nov 10, 2023

FYI, binaries are being prepared already, as it seemed likely that this would go ahead soon:
https://hydra.nixos.org/eval/1801549

Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay if it stays temporary. It's kind of funny how often something relating to C++ FFI breaks in Haskell exclusively on darwin…

# Works around https://github.com/NixOS/nixpkgs/issues/166205.
# This can be dropped once a fix has been committed to cc-wrapper.
// lib.optionalAttrs (stdenv.cc.isClang && stdenv.cc.libcxx != null) {
env.NIX_LDFLAGS = "-l${stdenv.cc.libcxx.cxxabi.libName}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This completely overwrites env if it is set. Is probably better to just set NIX_LDFLAGS top level to prevent confusion if this sticks around for longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I pushed an update that preserves env while also avoiding any further rebuilds (since the staging-next job on Hydra is currently pointed to a branch with this commit cherry-picked).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #266878 to preserve env since this got merged before I could push the update.

@vcunat vcunat merged commit 4f431bd into NixOS:staging-next Nov 11, 2023
21 checks passed
@vcunat
Copy link
Member

vcunat commented Nov 11, 2023

OK, merged like this right now. I suspect this hack will stay in 23.11, but I don't know if that would make it worth to improve this workaround during some other rebuild.

reckenrode added a commit to reckenrode/nixpkgs that referenced this pull request Nov 12, 2023
This is a follow-up PR to NixOS#266172 to address the feedback left by
@sternenseemann regarding `env`.

Rebuilds on staging-next NixOS#263535 should be limited if there are any.
sternenseemann added a commit to sternenseemann/nixpkgs that referenced this pull request Nov 17, 2023
If we don't check `hasCC`, we run into trouble on pkgsCross.ghcjs, one
of the few platforms where `cc` will throw
(so `stdenv.cc.isClang or false would` not be enough).

Problem introduced in 8164b19 / NixOS#266172.
sternenseemann added a commit that referenced this pull request Nov 17, 2023
If we don't check `hasCC`, we run into trouble on pkgsCross.ghcjs, one
of the few platforms where `cc` will throw
(so `stdenv.cc.isClang or false would` not be enough).

Problem introduced in 8164b19 / #266172.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants